Skip to content

[DT-4263][1/7] Record a send id on each email row - #3149

Merged
kevinmarete merged 5 commits into
developfrom
km-dt-4263-email-send-id
Oct 13, 2026
Merged

kevinmarete merged 5 commits into
developfrom
km-dt-4263-email-send-id

Conversation

@kevinmarete

@kevinmarete kevinmarete commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-4263

Security risk: low — adds a nullable send_id column to email_entity, filled with a random per-request UUID; no change to who receives email or who can read it.

Summary

email_entity has one row per recipient and nothing linking the rows of one send, so the email log (#3142) has to infer sends from a 10-minute gap between timestamps. Code review kept finding cases that inference gets wrong: overlapping batches, chains of repeat emails, sends split by the date range. This records the grouping when the emails are written instead.

  • A request filter gives each request a fresh send id, held in a ThreadLocal and cleared on the response. EmailService.sendMessage writes it on every row it inserts.
  • Every send loop runs inside one request, so one send is exactly the rows sharing a send_id, email type and entity reference, and no send loop has to change.
  • Emails written outside a request, and every existing row, keep send_id null. The email log will keep the current timestamp rule for those rows.
  • The one place email leaves the request thread is EmailNotifierResource.sendDailyMessages, which submits the expiration notices and both digests to an executor. Each of those jobs runs under its own send id through EmailSendId.run. The other executors (dashboards, dataset cleanup, reindexing) send no email.
  • Also pins testFetchByCreateDate_with_limit_and_offset to UTC: it read the local date but built UTC midnights, so it failed between 8pm and midnight Eastern.

#3142 will switch its grouping to send_id where it's set once this merges.


Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

🤖 Generated with Claude Code

@kevinmarete kevinmarete self-assigned this Oct 10, 2026
@kevinmarete kevinmarete changed the title [DT-4263] Record a send id on each email row [DT-4263][1/7] Record a send id on each email row Oct 10, 2026
@kevinmarete
kevinmarete marked this pull request as ready for review October 10, 2026 05:31
@kevinmarete
kevinmarete requested a review from a team as a code owner October 10, 2026 05:31
@kevinmarete
kevinmarete requested review from otchet-broad and rushtong and a balanced review from Copilot and removed request for a team October 10, 2026 05:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kevinmarete
kevinmarete requested a review from fboulnois October 10, 2026 05:31

@otchet-broad otchet-broad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I've left a note in a test class about coverage when a filter isn't registered (tests still pass) that can be addressed as you see fit.

@rushtong rushtong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. No medium or high findings:

  • The EmailSendId ThreadLocal clears only in the response filter. Dropwizard reduces this risk.
  • The new send_id column has no index. Later PRs in the stack look up rows by send_id, so they need one.
  • An async email sender that is not wrapped in EmailSendId.run writes a null send_id.
  • No DAO test inserts a null sendId and then reads back send_id IS NULL. testInsert_stores_the_send_id tests only a non-null UUID. The service test for null uses a mock DAO, so it does not use the database. This is a small gap, because the null inserts in the other DAO tests pass.

@fboulnois fboulnois left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 👍

@kevinmarete

Copy link
Copy Markdown
Contributor Author

@rushtong added the null send_id DAO test in 6fb44a7. No index on send_id: the sends query reads by create_date and only partitions by send_id within that range, so there's no lookup for an index to serve.

kevinmarete and others added 5 commits October 13, 2026 14:02
Every email a request writes now carries that request's send_id, so
the email log can group a send exactly instead of inferring it from
timestamps. Emails written outside a request, and every row before
this change, keep a null send_id and fall back to the existing rules.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sendDailyMessages runs the expiration notices and both digests on an
executor, outside the request filter, so those rows were written with
no send id. Each job now runs under its own through EmailSendId.run.
Also gives the filter an explicit priority, and drops the constructor
that let an insert omit the send id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kevinmarete
kevinmarete force-pushed the km-dt-4263-email-send-id branch from 6fb44a7 to a996af7 Compare October 13, 2026 18:04
@sonarqubecloud

Copy link
Copy Markdown

@kevinmarete
kevinmarete merged commit e3b7265 into develop Oct 13, 2026
11 checks passed
@kevinmarete
kevinmarete deleted the km-dt-4263-email-send-id branch October 13, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants